Fix rapid changes to colored_status_led#2938
Conversation
…. prior to this fix, setting color/on/off in rapid succession would just send the updated values "down the string" Whilst it would be nice to have the PIO program take care of updating the latest FIFO value after the relevant delay, that costs a fair amount of PIO instrution space, so I have decided to "fix" the issue on the CPU side. Since the delays in question are non trivial (e.g. 50us), i've decided not to have the call block, but instead set a timer when necessary (using the default alarm pool) - if that isn't available or the alarm add fails, then the caller blocks instead. Note the implementation uses a spin lock, because it needs to work with multicore, but also to be fair you might expect to be able to set an LED from an IRQ
|
|
||
| // PICO_CONFIG: PICO_COLORED_STATUS_LED_RESET_DELAY_US, Required reset delay in microseconds for the WS2812 colored status LED , type=int, default=50, group=pico_status_led | ||
| #ifndef PICO_COLORED_STATUS_LED_RESET_DELAY_US | ||
| #define PICO_COLORED_STATUS_LED_RESET_DELAY_US 50 |
There was a problem hiding this comment.
This define doesn't seem to actually be used anywhere??
There was a problem hiding this comment.
Yes, the calculated MINIMUM_WS2812_DELAY_US seems to be used instead.
If it wants to be configurable, could use #define PICO_COLORED_STATUS_LED_RESET_CYCLES which defaults to PICO_COLORED_STATUS_LED_USES_WRGB ? 32 : 24, and then use PICO_COLORED_STATUS_LED_RESET_CYCLES in the MINIMUM_WS2812_DELAY_US calculation?
Equally, could just remove this define, if the value is expected to be fixed
There was a problem hiding this comment.
yeah, was a bug - they are indeed supposed to be added together
|
I guess the PR addresses this comment 🙂 |
| #define alarm_id 0 | ||
| #endif | ||
|
|
||
| #define MINIMUM_WS2812_DELAY_US (1 + (1000000 * (PICO_COLORED_STATUS_LED_USES_WRGB ? 32 : 24)) / PICO_COLORED_STATUS_LED_WS2812_FREQ) |
There was a problem hiding this comment.
All the boards I have on my desk require 3x this delay value to work correctly - where did this value come from?
My test is just a modified color_blink example to send colours as often as possible - when working it is white (ie cycling through red-green-blue), when not working it is just red (the first colour set)
#include "pico/stdlib.h"
#include "pico/status_led.h"
int main() {
status_led_init();
uint32_t count = 0;
while (true) {
// flash red then green then blue
uint32_t color = PICO_COLORED_STATUS_LED_COLOR_FROM_RGB(count % 3 == 0 ? 0xaa : 0, count % 3 == 1 ? 0xaa : 0, count % 3 == 2 ? 0xaa : 0);
colored_status_led_set_on_with_color(color);
count++;
}
status_led_deinit();
}There was a problem hiding this comment.
weird ok - i mean 50us is what the WS2812 datasheet says - i'm fine with 150
There was a problem hiding this comment.
actually i changed it to 100 as we weren't including the actual color setting time before
Co-authored-by: Andrew Scheller <lurch@durge.org>
- actually include the color setting time in the caluclation - split out non alarm pool code for clarity - fix cleanup code and return value from set_ws2812 (we guess if we are deferring!)
3 participants
WS2812 style LEDs require a reset time before setting the color againprior to this fix, setting color/on/off
in rapid succession would just send the updated values "down the string"
Whilst it would be nice to have the PIO program take care of updating the latest FIFO value after the relevant delay, that costs a fair amount of PIO instrution space, so I have decided to "fix" the issue on the CPU side.
Since the delays in question are non trivial (e.g. 50us), i've decided not to have the call block, but instead set a timer when necessary (using the default alarm pool) - if that isn't available or the alarm add fails, then the caller blocks instead.
Note the implementation uses a spin lock, because it needs to work with multicore, but also to be fair you might expect to be able to set an LED from an IRQ